Memory lru #695

gsoltis · 2018-01-23T21:56:10Z

Do Not Merge.

This PR shows an implementation of FSTLRUGarbageCollector. The functioning of that GC strategy requires a few changes to FSTQueryCache.h, which are shown here and implemented for FSTMemoryQueryCache.

Tests are provided for various steps in the process of running the LRU GC, however the GC is not wired into the Local Store and will not yet be triggered. I expect that the tests will run against LevelDB persistence which minimal modifications once I have implemented the necessary pieces in FSTLevelDBQueryCache and any other necessary places.

There are two notable changes beyond the LRU implementation and associated query cache changes:

The local store informs the QueryCache of documents that have been written at the time when they are added to the RemoteDocumentCache.
FSTMemoryQueryCache now tracks documents that have been written from the point when they are ack'd until the point where they are added to a query target. The second part is an optimization, we could theoretically keep them even after they appear in a target.

Not yet implemented:

LevelDB anything. Haven't touched it yet
Refreshing sequence numbers for queries after receiving resume tokens.
Handling documents removed from a query in local store

wilhuff · 2018-01-25T16:36:59Z

Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.m

+ * limitations under the License.
+ */
+
+#import <XCTest/XCTest.h>


nit: separate framework imports from the rest

wilhuff · 2018-01-25T16:41:27Z

Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.m

+}
+
+- (void)testPickSequenceNumberPercentile {
+  const int numTestCases = 5;


Note you can define a struct locally here within this function and initialize it just as you've done. Something like this would allow you to avoid the fiddly indexes in the code below.

struct Case { int queries; int expected; }; Case testCases[numTestCases] = {{0, 0}, {10, 1}, {9, 0}, {50, 5}, {49, 4}};

Cool, I didn't know you could just define structs wherever. I definitely like that better.

wilhuff · 2018-01-25T16:43:31Z

Firestore/Source/Local/FSTLRUGarbageCollector.mm

@@ -0,0 +1,101 @@
+#import "Firestore/Source/Local/FSTLRUGarbageCollector.h"
+#import <queue>


newline before

wilhuff · 2018-01-25T16:46:29Z

Firestore/Source/Local/FSTLRUGarbageCollector.mm

+class RollingSequenceNumberBuffer {
+ public:
+  RollingSequenceNumberBuffer(NSUInteger max_elements) : max_elements_(max_elements) {
+    queue_ = std::make_unique<priority_queue<FSTListenSequenceNumber> >();


Since you're not letting this priority queue escape or otherwise be shared you don't need to heap allocate it.

Just make the member

std::priority_queue<FSTListenceSequenceNumber> queue_;

Then you can initialize it exactly as you have max_elements_.

Done. In the future, I think I can even stack allocate the backing vector, since I know the max size at constructor time.

wilhuff · 2018-01-25T16:48:02Z

Firestore/Source/Local/FSTLRUGarbageCollector.mm

+
+const FSTListenSequenceNumber kFSTListenSequenceNumberInvalid = -1;
+
+using std::priority_queue;


I'm kind of meh on using statements for members of std. If you're going to use them, use them consistently.

Ok, removed the using.

wilhuff · 2018-01-25T17:00:02Z

Firestore/Source/Local/FSTLRUGarbageCollector.mm

+    queue_ = std::make_unique<priority_queue<FSTListenSequenceNumber> >();
+  }
+
+  RollingSequenceNumberBuffer(const RollingSequenceNumberBuffer &other) = delete;


You only need to define one copy constructor and assignment operator as deleted to suppress the other.

You can prove it to yourself with a snippet like the following (though please delete after):

static_assert(!std::is_move_constructable<RollingSequenceNumberBuffer>::type, "no moves"); static_assert(!std::is_move_assignable<RollingSequenceNumberBuffer>::type, "no move assignments"); static_assert(!std::is_copy_constructable<RollingSequenceNumberBuffer>::type, "no copies"); static_assert(!std::is_copy_assignable<RollingSequenceNumberBuffer>::type, "no copy assignments");

https://stackoverflow.com/questions/33776697/deleting-copy-constructors-and-copy-assignment-operators-which-of-them-are-esse

In the recommended fashion, I was just copying what I saw elsewhere. Any difference / preference for which one of each is kept?

For reference: https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/core/src/firebase/firestore/remote/datastore.h

Do what I say, not what I do :-P.

wilhuff · 2018-01-25T17:04:55Z

Firestore/Source/Local/FSTLocalStore.m

@@ -418,6 +421,8 @@ - (void)releaseQuery:(FSTQuery *)query {
  FSTAssert(queryData, @"Tried to release nonexistent query: %@", query);

  [self.localViewReferences removeReferencesForID:queryData.targetID];
+  // TODO(gsoltis): kill this if statement, give GC a reference to query cache,


The definitely sounds right.

wilhuff · 2018-02-20T07:49:40Z

Firestore/Source/Local/FSTLRUGarbageCollector.h

@@ -0,0 +1,29 @@
+#import <Foundation/Foundation.h>


Missing copyright (and that's why Travis failed your build).

wilhuff · 2018-02-20T07:50:41Z

Firestore/Source/Local/FSTLRUGarbageCollector.mm

+
+@end
+
+@implementation FSTLRUGarbageCollector {


The empty block here is unnecessary.

wilhuff · 2018-02-20T07:51:19Z

Firestore/Source/Local/FSTLRUGarbageCollector.mm

+
+const FSTListenSequenceNumber kFSTListenSequenceNumberInvalid = -1;
+
+class RollingSequenceNumberBuffer {


Comment on the purpose of the class.

In particular it's helpful to understand why a priority queue that efficiently tracks the highest seen sequence number is helpful. I think it's because you're looking for the oldest (i.e. smallest) max_elements number of sequence numbers, right?

wilhuff · 2018-02-20T08:02:13Z

Firestore/Source/Local/FSTLRUGarbageCollector.mm

+      : max_elements_(max_elements), queue_(std::priority_queue<FSTListenSequenceNumber>()) {
+  }
+
+  RollingSequenceNumberBuffer(const RollingSequenceNumberBuffer &other) = delete;


If you declare any copy constructor/assignment that implicitly precludes the move operations from being generated. The deletions of the moves are redundant.

https://stackoverflow.com/questions/33096653/make-a-class-non-copyable-and-non-movable

wilhuff · 2018-02-20T08:02:35Z

Firestore/Source/Local/FSTLRUGarbageCollector.mm

@@ -0,0 +1,100 @@
+#import "Firestore/Source/Local/FSTLRUGarbageCollector.h"


wilhuff · 2018-02-20T08:24:19Z

Firestore/Source/Local/FSTMemoryQueryCache.mm

+          }
+        }
+      }];
+  [self.queries removeObjectsForKeys:toRemove];


Do we also need to remove document keys from the mutated document sequence numbers?

wilhuff · 2018-02-20T08:26:19Z

Firestore/Source/Local/FSTQueryCache.h

@@ -98,6 +98,18 @@ NS_ASSUME_NONNULL_BEGIN
 /** Removes the cached entry for the given query data (no-op if no entry exists). */
 - (void)removeQueryData:(FSTQueryData *)queryData group:(FSTWriteGroup *)group;

+- (void)enumerateSequenceNumbersUsingBlock:(void (^)(FSTListenSequenceNumber sequenceNumber,


This should note that it's not obligated to return sequence numbers in order.

wilhuff · 2018-02-20T08:29:24Z

Firestore/Source/Local/FSTQueryCache.h

+- (void)enumerateSequenceNumbersUsingBlock:(void (^)(FSTListenSequenceNumber sequenceNumber,
+                                                     BOOL *stop))block;
+
+- (NSUInteger)removeQueriesThroughSequenceNumber:(FSTListenSequenceNumber)sequenceNumber


Comments. Removes queries with sequence numbers up to and including the given sequence number.

What does live queries have to do with it? Are they excluded? Does encountering a live query terminate the removal?

wilhuff · 2018-02-20T08:31:53Z

Firestore/Source/Local/FSTMemoryQueryCache.mm

+  NSMutableArray<FSTQuery *> *toRemove = [NSMutableArray array];
+  [self.queries
+      enumerateKeysAndObjectsUsingBlock:^(FSTQuery *query, FSTQueryData *queryData, BOOL *stop) {
+        if (queryData.sequenceNumber <= sequenceNumber) {


Can't this be a compound condition separated by &&?

wilhuff · 2018-02-20T08:38:24Z

Firestore/Source/Local/FSTMemoryQueryCache.mm

+                                           group:(__unused FSTWriteGroup *)group {
+  NSMutableArray<FSTQuery *> *toRemove = [NSMutableArray array];
+  [self.queries
+      enumerateKeysAndObjectsUsingBlock:^(FSTQuery *query, FSTQueryData *queryData, BOOL *stop) {


I think you can avoid this second scan if your priority were to track (sequence number, target id) pairs. Order by the first (which yields the same sequence numbers) but then once you're done accumulating the queue you have all the primary keys of the targets to delete in hand.

You'll need to store something else to indicate it was a mutated document key instead; not sure if this is required (see other questions).

gsoltis · 2018-02-20T16:59:50Z

Sorry, this wasn't intended to be reviewed yet. I'll assign someone when it is. I'd like to get a review of another PR first before tackling this one. I will hopefully have that one up today or tomorrow.

ryanwilson · 2018-12-13T14:04:19Z

@gsoltis is this PR abandoned? If so, we should close it.

gsoltis · 2018-12-13T19:16:01Z

Yup, a different PR was merged that included this. Closing.

Greg Soltis added 9 commits January 18, 2018 16:36

Start implementing percentile and sequence number calculations

941c74c

C++\!

0a85568

Clear queryData

a64cb8e

Most of the meat of LRU GC done for memory persistence

65a2a30

Fix header include

bd8e9f9

Auto-styling

fa31549

Add mutated document tracking

41ee4c9

Style

e7d5f86

Drop changes to FSTReferenceSet

a5b2d17

gsoltis self-assigned this Jan 23, 2018

gsoltis requested review from mikelehen, wilhuff and schmidt-sebastian January 23, 2018 21:56

googlebot added the cla: yes label Jan 23, 2018

wilhuff reviewed Jan 25, 2018

View reviewed changes

Greg Soltis added 15 commits January 25, 2018 10:38

Review feedback

b6559c3

Automated style fixes

4890398

Implement schema versions

60d36af

Style fixes

52ad8cc

newlines, copyrights, assumptions

93a84b1

Fix nullability

0580b9a

Raw ptr -> shared_ptr

77e4ed2

kVersionTableGlobal -> kVersionGlobalTable

d3f269c

Drop utils, move into static methods

a6fa651

Drop extra include

578a020

Add a few more comments

d7290cc

Move version constant into migrations file

9dfd89d

formatting?

ba24516

Fix comment

638a9d5

Split add and update queryData

09ac0d3

Greg Soltis added 10 commits January 30, 2018 14:42

Work on adding targetCount

2019e8d

More work on count

55ec377

Using shared_ptr

8ecba99

merging in master

f2308ef

Implement count for query cache

87767f6

Merge branch 'master' into count_queries

187fc36

use quotes

1636f59

Merge in master

8e51477

Fixup

e38bf80

Merging in master

b5c47d9

wilhuff reviewed Feb 20, 2018

View reviewed changes

mikelehen removed their request for review April 11, 2018 23:25

wilhuff added the api: firestore label Apr 12, 2018

gsoltis closed this Dec 13, 2018

gsoltis deleted the memory_lru branch December 13, 2018 19:16

firebase locked and limited conversation to collaborators Nov 8, 2019

		@@ -0,0 +1,101 @@
		#import "Firestore/Source/Local/FSTLRUGarbageCollector.h"
		#import <queue>


		const FSTListenSequenceNumber kFSTListenSequenceNumberInvalid = -1;

		using std::priority_queue;


		const FSTListenSequenceNumber kFSTListenSequenceNumberInvalid = -1;

		class RollingSequenceNumberBuffer {

		@@ -0,0 +1,100 @@
		#import "Firestore/Source/Local/FSTLRUGarbageCollector.h"


		@end

		@implementation FSTLRUGarbageCollector {

Memory lru #695

Memory lru #695

Uh oh!

Conversation

gsoltis commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsoltis commented Feb 20, 2018

Uh oh!

ryanwilson commented Dec 13, 2018

Uh oh!

gsoltis commented Dec 13, 2018

Uh oh!

Uh oh!